Skip to content

Conversation

@predragnikolic
Copy link
Member

closes #2

@jfcherng
Copy link
Contributor

jfcherng commented Dec 26, 2019

I was using many other scopes/syntaxes with jsonc. Maybe you will be interested in

scopes

"source.json.settings",
"source.json.sublime.build",
"source.json.sublime.color",
"source.json.sublime.commands",
"source.json.sublime.completions",
"source.json.sublime.keymap",
"source.json.sublime.macro",
"source.json.sublime.menu",
"source.json.sublime.mousemap",
"source.json.sublime.project",
"source.json.sublime.settings",
"source.json.sublime.theme",

sytaxes

"Packages/PackageDev/Package/Sublime JSON/Sublime JSON.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Build System/Sublime Text Build System.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Color Scheme/Sublime Text Color Scheme.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Commands/Sublime Text Commands.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Completions/Sublime Text Completions.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Keymap/Sublime Text Keymap.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Macro/Sublime Text Macro.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Menu/Sublime Text Menu.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Mousemap/Sublime Text Mousemap.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Project/Sublime Text Project.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Settings/Sublime Text Settings.sublime-syntax",
"Packages/PackageDev/Package/Sublime Text Theme/Sublime Text Theme.sublime-syntax",
"Packages/PackageDev/Package/TextMate Syntax Definition (JSON)/TextMate Syntax Definition (JSON).sublime-syntax",

@predragnikolic
Copy link
Member Author

I was thinking if we should add all those syntaxes.

Most people will mostly encounter only sublime settings, keymap and sublime project files.
In that case it does make sense to only have those scopes configured. :)
On the other hand, if we open a *.sublime-menu or *.sublime-build file we know we want to start a jsonc server.
So it does make sense to start the server on these files.

I think we should add them. :)

@predragnikolic
Copy link
Member Author

I should probably mention in the README that the PackageDev package needs to be installed.
Because without it, non of the above scopes exist (source.json.sublime...)
and opening the sublime settings file will start a regular json server, which would then report errors for comments.

@predragnikolic
Copy link
Member Author

Just so you know, I haven't forgotten about this PR.

I was looking at a way to not have PackageDev as a requirement for LSP-json to work. The replacement are JSON schema files... :)

JSON schemas will give the possibility to have autocomplete in other plugin settings files as long as the have a defined schema. :)

LSP-json will have its own JSONC syntax file that will come with this package, and will be assigned to all sublime, settings, theme, completions, keymaps, files... So no need to have 10 different scopes in the config for jsonc. There will be just one and that is source.jsonc.

Here is an example of how it works.
https://user-images.githubusercontent.com/22029477/71563766-18a23a00-2a95-11ea-9868-8a198c13870c.gif

I am still working on this, so expect something like this in the future.

@@ -0,0 +1,143 @@
%YAML 1.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of this sublime-syntax file?

Copy link
Contributor

@jfcherng jfcherng Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think He doesn't want this plugin to depend on PackageDev so he creates it for sublime-related files. Otherwise, users can directly use #6 (comment) in their settings to activate LSP-json.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the point.

LSP-json will have its own JSONC syntax file that will come with this package, and will be assigned to all sublime, settings, theme, completions, keymaps, files... So no need to have 10 different scopes in the config for jsonc. There will be just one and that is source.jsonc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it :( The PackageDev syntaxes have great highlighting. What's more is that there is already a syntax for JSON with comments at Packages/JavaScript/JSON.sublime-syntax.
Why does this plugin need to depend on PackageDev in the first place?
Also, why not write a language server for json-with-comments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not put jsonc instead of json for the languageId? Wouldn't that make the json-language-server shut up about comments? https://github.com/sublimelsp/LSP-json/blob/master/plugin.py#L85

https://github.com/vscode-langservers/vscode-json-languageserver#server-capabilities

jsonc documents additionally accept single line (//) and multi-line comments (/* ... */).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now that this pull request modifies the languageId from json to jsonc. So that should already remove the errors about comments. So then why not use Packages/JavaScript/JSON.sublime-syntax? Remember that maintaining a custom JSON syntax is some serious maintainer burden...

Copy link
Contributor

@jfcherng jfcherng Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember that maintaining a custom JSON syntax is some serious maintainer burden...

I don't like this solution either (maybe because I have PackageDev installed :D ), but...

So then why not use Packages/JavaScript/JSON.sublime-syntax?

ST's JSON doesn't differentiate json and jsonc since it treats everything as jsonc. If we do so, LSP will not treat a comma (and comment) in a JSON file as an error. If we want a way to differentiate them, then either via a 3rd-party (PackageDev) or creates our own...


Another way to not actually maintain the syntax "definition" is something like:

%YAML 1.2
---
name: JSONC
file_extensions:
  - jsonc
  - sublime-settings
  - sublime-menu
  - sublime-keymap
  - sublime-mousemap
  - sublime-theme
  - sublime-build
  - sublime-project
  - sublime-completions
  - sublime-commands
  - sublime-macro
  - sublime-color-scheme
  - ipynb
  - Pipfile.lock
scope: source.jsonc
contexts:
  prototype:
    - include: Packages/JavaScript/JSON.sublime-syntax#prototype
  main:
    - include: Packages/JavaScript/JSON.sublime-syntax#main

It results in a base scope source.jsonc with other ....json subscopes.

image

But there is no guarantee that ST will chooses your syntax definition over PackageDev's (or other 3rd-party plugin's). I don't know how ST chooses the syntax when there are multiple qualified candidates.

@rchl
Copy link
Member

rchl commented Mar 14, 2020

What about having an alternative, glob-based activation of the language server? So for this server configuration would look something like:

"languages": [
  {
    "languageId": "json",
    "scopes": ["source.json"],
    "syntaxes": [
      "Packages/JavaScript/JSON.sublime-syntax",
      "Packages/JSON/JSON.sublime-syntax"
    ],
  },
  {
    "languageId": "jsonc",
    "scopes": ["source.json"],
    "path_match": [
      "*.sublime_settings",
      "*.sublime_keymap"
    ]
  }
]
  • Since the scopes are overlapping, the configuration with path_match would have to have priority when matching.
  • Since it requires path/file name, it would not work for unsaved files I guess.

@predragnikolic
Copy link
Member Author

I will close this PR in favor of #8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove support for sublime-settings

5 participants